-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove leftover _eventAdapter
from EdrProviderWrapper
#5400
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Seems to be a leftover from #4747 and it looks to be unused now as we create a new instance of `EdrProviderEventAdapter` explicitly in `create()` and move it into lambda. Feel free to disregard if that's not true and/or we want to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@fvictorio can I get a double check that it's also not used externally like |
Uhm, I'm not sure we can remove this. This might cause I'm not sure at all if that could really happen, because this things are always tricky in javascript, but I'd say we err on the side of paranoia and keep it. We should add a comment about this in the field though. |
@Wodann do you remember if that's the reason you added this to |
In theory it should always be traced (in the GC sense) through the lambda and thus kept alive but I see now that the subscriber callback is moved to the Rust side, so it depends on the fact how it's implemented exactly. From the cursory glance it seems that:
which leads me to believe that it should be reachable by v8 and thus kept alive; I'd be really surprised if the func argument to Going a bit deeper into the node internals:
The last part is a bit hand-wave-y as I don't want to get so deep into how exactly environment/resource management is implemented (there's a lot of ground and minutiae to cover!) All in all, this looks safe to do. WDYT? |
Yes, overall it seems like a safe bet. I'm not sure how this would manifest if our assumption is wrong, but I think it would mean that filters would stop working "after a while". Worth keeping in mind in case we see that behavior in the future, although it does seem highly unlikely. |
Seems to be a leftover from #4747 and it looks to be unused now as we create a new instance of
EdrProviderEventAdapter
explicitly increate()
and move it into lambda.Feel free to disregard if that's not true and/or we want to keep it.